Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

default session saving completed & tested #156

Merged
merged 17 commits into from
Jan 19, 2024
Merged

default session saving completed & tested #156

merged 17 commits into from
Jan 19, 2024

Conversation

gigajuwels
Copy link
Contributor

No description provided.

@gigajuwels gigajuwels requested a review from skibitsky November 12, 2023 18:45
skibitsky
skibitsky requested changes Nov 13, 2023
View reviewed changes
Copy link
Member

@skibitsky skibitsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good! Just a few small requests and questions

Core Modules/WalletConnectSharp.Storage/InMemoryStorage.cs Outdated Show resolved Hide resolved
Tests/WalletConnectSharp.Sign.Test/SignTests.cs Show resolved Hide resolved
WalletConnectSharp.Core/Controllers/Relayer.cs Outdated Show resolved Hide resolved
WalletConnectSharp.Core/Models/MessageHandler/TypedEventHandler.cs Outdated Show resolved Hide resolved
WalletConnectSharp.Sign/Models/DefaultsLoadingEventArgs.cs Outdated Show resolved Hide resolved
WalletConnectSharp.Sign/Controllers/AddressProvider.cs Outdated Show resolved Hide resolved
WalletConnectSharp.Sign/Controllers/AddressProvider.cs Show resolved Hide resolved
WalletConnectSharp.Sign/Controllers/AddressProvider.cs Outdated Show resolved Hide resolved
WalletConnectSharp.Sign/Models/SessionRequestEventHandler.cs Outdated Show resolved Hide resolved
WalletConnectSharp.Sign/Models/SessionRequestEventHandler.cs Outdated Show resolved Hide resolved
@skibitsky
Copy link
Member

There is another issue that can break session requests.

Engine now uses OrderedKeys field when making requests:
https://github.com/WalletConnect/WalletConnectSharp/blob/feat/default-session-saving/WalletConnectSharp.Sign/Engine.cs#L687

The problem I'm sometimes facing is that RequiredNamespace object can have one Key, but no OrderedKeys. Perhaps there is a scenario when dictionary is assigned directly, bypassing the Add method of RequiredNamespaces.

@skibitsky skibitsky mentioned this pull request Nov 24, 2023
Closed
@gigajuwels gigajuwels requested a review from skibitsky November 29, 2023 17:03
@skibitsky
Copy link
Member

@gigajuwels the TestAddressProviderDefaultsSaving test fails

gigajuwels and others added 9 commits December 9, 2023 18:13
@gigajuwels
fix: dispose loop
27cd591
@gigajuwels
feat: AddressProvider uses Namespaces instead of RequiredNamespaces
55b210c 
also introduce SortedDictionary
@gigajuwels
feat: added relay batch message fetching and fixed several lifecycle …
60d5fa9 
…issues
@gigajuwels
test: Ensure namespaces include Chains array
77c03d3
@gigajuwels
chore: remove debug logging
dac3bfb
@gigajuwels
chore: replace foreach await with task.whenall
666aac4
@skibitsky
Make udpated FileSystemStorage backwards compatibile
878b893
@skibitsky
Fix namespaces not being deserialized correctly
2ea8d21 
`OrderedKeys` has always been 0 when deserializing the Namespaces JSON. The issue was solved by replacing `SortedDictionary` with an implementation from `System.Collections.Generic`.
@skibitsky
Merge branch '2.0' into feat/default-session-saving
758b561 
# Conflicts:
#	Core Modules/WalletConnectSharp.Storage/FileSystemStorage.cs
@skibitsky
Copy link
Member

Well done, @gigajuwels!

@skibitsky skibitsky merged commit ec81740 into 2.0 Jan 19, 2024
3 checks passed
@skibitsky skibitsky mentioned this pull request Feb 22, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skibitsky skibitsky skibitsky approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gigajuwels @skibitsky